-
Notifications
You must be signed in to change notification settings - Fork 4
asynch export network #3285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
asynch export network #3285
Conversation
import { RestoreNodesDialog } from './dialogs/restore-node-dialog'; | ||
import { CopyType } from './network-modification.type'; | ||
import { NodeSequenceType, NotificationType, PENDING_MODIFICATION_NOTIFICATION_TYPES } from 'types/notification-types'; | ||
import useExportSubscription from '../hooks/use-export-subscription.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import useExportSubscription from '../hooks/use-export-subscription.js'; | |
import useExportSubscription from '../hooks/use-export-subscription'; |
sendAlert(eventData); | ||
return; // here, we do not want to update the redux state | ||
} | ||
if (isExportNetworkNotification(eventData)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sylvain has a ticket in this sprint to remove studyUpdated to redux.
In the future, each callback handler must subscribe separately by useNotificationListener and inside each handler we check the updateType, for example in yours case, using predicate isExportNetworkNotification
} | ||
} | ||
|
||
export default function useExportNotificationHandler() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export default function useExportNotificationHandler() { | |
export default function useExportNotification() { |
[userId, snackWarning, downloadExportNetworkFile, snackInfo, intl] | ||
); | ||
|
||
return { handleExportNotification }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { handleExportNotification }; | |
useNotificationsListener(NotificationsUrlKeys.STUDY, { listenerCallbackMessage: handleExportNotification }); | |
const { | ||
studyUuid, | ||
node, | ||
rootNetworkUuid, | ||
format, | ||
userId: useId, | ||
exportUuid, | ||
fileName, | ||
error, | ||
} = eventData.headers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { | |
studyUuid, | |
node, | |
rootNetworkUuid, | |
format, | |
userId: useId, | |
exportUuid, | |
fileName, | |
error, | |
} = eventData.headers; | |
const eventData = JSON.parse(event.data); | |
if (isExportNetworkNotification(eventData)) { | |
const { | |
studyUuid, | |
node, | |
rootNetworkUuid, | |
format, | |
userId: useId, | |
exportUuid, | |
fileName, | |
error, | |
} = eventData.headers; | |
// old code | |
} |
[dispatch, displayErrorNotifications, handleExportNotification, sendAlert] | ||
); | ||
|
||
useNotificationsListener(NotificationsUrlKeys.STUDY, { listenerCallbackMessage: handleStudyUpdate }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useExportNotification();
const userId = useSelector((state: AppState) => state.user?.profile.sub); | ||
|
||
const handleExportNotification = useCallback( | ||
(eventData: ExportNetworkEventData) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(eventData: ExportNetworkEventData) => { | |
(event: MessageEvent<string>) => { |
|
||
const { subscribeExport } = useExportSubscription({ | ||
studyUuid: studyUuid, | ||
nodeUuid: currentNode?.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must pass nodeUuid in the params of subscribeExport because it is activeNode.id not currentNode.id
BUG when a node is not selected but we do export on this node via context menu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three files use-export-xxx can be grouped in a file unique which presents a feature or move all into a directory, for example ./use-export-network
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have create a PR which corrige following my comments except grouping hook files
|
No description provided.